Handle misformatted versions with a nicer error message.

diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs
index 40a30aadf758dbe6f219d7df13ca9ef80e97ebbd..d96e21547dfd985e10df6e06f6da50dbfc80b7c1 100644 (file)
--- a/src/cargo/core/package_id.rs
+++ b/src/cargo/core/package_id.rs
@@ -10,43 +10,46 @@ use serialize::{
     Decoder
 };
 
-use util::{CargoError, FromError};
+use util::{CargoResult, CargoError};
 
 trait ToVersion {
-    fn to_version(self) -> Option<semver::Version>;
+    fn to_version(self) -> Result<semver::Version, String>;
 }
 
 impl ToVersion for semver::Version {
-    fn to_version(self) -> Option<semver::Version> {
-        Some(self)
+    fn to_version(self) -> Result<semver::Version, String> {
+        Ok(self)
     }
 }
 
 impl<'a> ToVersion for &'a str {
-    fn to_version(self) -> Option<semver::Version> {
-        semver::parse(self)
+    fn to_version(self) -> Result<semver::Version, String> {
+        match semver::parse(self) {
+            Some(v) => Ok(v),
+            None => Err(format!("cannot parse '{}' as a semver", self)),
+        }
     }
 }
 
 trait ToUrl {
-    fn to_url(self) -> Option<Url>;
+    fn to_url(self) -> Result<Url, String>;
 }
 
 impl<'a> ToUrl for &'a str {
-    fn to_url(self) -> Option<Url> {
-        url::from_str(self).ok()
+    fn to_url(self) -> Result<Url, String> {
+        url::from_str(self)
     }
 }
 
 impl ToUrl for Url {
-    fn to_url(self) -> Option<Url> {
-        Some(self)
+    fn to_url(self) -> Result<Url, String> {
+        Ok(self)
     }
 }
 
 impl<'a> ToUrl for &'a Url {
-    fn to_url(self) -> Option<Url> {
-        Some(self.clone())
+    fn to_url(self) -> Result<Url, String> {
+        Ok(self.clone())
     }
 }
 
@@ -57,14 +60,32 @@ pub struct PackageId {
     namespace: Url
 }
 
+#[deriving(Clone, Show, PartialEq)]
+pub enum PackageIdError {
+    InvalidVersion(String),
+    InvalidNamespace(String)
+}
+
+impl CargoError for PackageIdError {
+    fn description(&self) -> String {
+        match *self {
+            InvalidVersion(ref v) => format!("invalid version: {}", *v),
+            InvalidNamespace(ref ns) => format!("invalid namespace: {}", *ns),
+        }
+    }
+    fn is_human(&self) -> bool { true }
+}
+
 impl PackageId {
     pub fn new<T: ToVersion, U: ToUrl>(name: &str, version: T,
-                                       namespace: U) -> PackageId {
-        PackageId {
+                                       namespace: U) -> CargoResult<PackageId> {
+        let v = try!(version.to_version().map_err(InvalidVersion));
+        let ns = try!(namespace.to_url().map_err(InvalidNamespace));
+        Ok(PackageId {
             name: name.to_str(),
-            version: version.to_version().unwrap(),
-            namespace: namespace.to_url().unwrap()
-        }
+            version: v,
+            namespace: ns
+        })
     }
 
     pub fn get_name<'a>(&'a self) -> &'a str {
@@ -94,14 +115,17 @@ impl Show for PackageId {
     }
 }
 
-impl<E: CargoError + FromError<E>, D: Decoder<E>> Decodable<D,E> for PackageId {
-    fn decode(d: &mut D) -> Result<PackageId, E> {
-        let vector: Vec<String> = try!(Decodable::decode(d));
+impl<D: Decoder<Box<CargoError>>> Decodable<D,Box<CargoError>> for PackageId {
+    fn decode(d: &mut D) -> Result<PackageId, Box<CargoError>> {
+        let vector: Vec<String> = match Decodable::decode(d) {
+            Ok(v) => v,
+            Err(e) => return Err(e.to_error())
+        };
 
-        Ok(PackageId::new(
+        PackageId::new(
             vector.get(0).as_slice(),
             vector.get(1).as_slice(),
-            vector.get(2).as_slice()))
+            vector.get(2).as_slice())
     }
 }
 
@@ -111,3 +135,16 @@ impl<E, S: Encoder<E>> Encodable<S,E> for PackageId {
               self.namespace.to_str()).encode(e)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{PackageId, central_repo};
+
+    #[test]
+    fn invalid_version_handled_nicely() {
+        assert!(PackageId::new("foo", "1.0", central_repo).is_err());
+        assert!(PackageId::new("foo", "1", central_repo).is_err());
+        assert!(PackageId::new("foo", "bar", central_repo).is_err());
+        assert!(PackageId::new("foo", "", central_repo).is_err());
+    }
+}

diff --git a/src/cargo/core/resolver.rs b/src/cargo/core/resolver.rs
index e7d04427f15def437279d836136716ba7878d293..da0dcb834bf2f91c083c9cb5fe51920f76521553 100644 (file)
--- a/src/cargo/core/resolver.rs
+++ b/src/cargo/core/resolver.rs
@@ -88,19 +88,19 @@ mod test {
                 Dependency::parse(*s, Some("1.0.0"), &source_id).unwrap()
             }).collect();
             Summary::new(&PackageId::new($name, "1.0.0",
-                                         "http://www.example.com/"),
+                                         "http://www.example.com/").unwrap(),
                          d.as_slice())
             }
         );
 
         ($name:expr) => (
             Summary::new(&PackageId::new($name, "1.0.0",
-                                         "http://www.example.com/"), [])
+                                         "http://www.example.com/").unwrap(), [])
         )
     )
 
     fn pkg(name: &str) -> Summary {
-        Summary::new(&PackageId::new(name, "1.0.0", "http://www.example.com/"),
+        Summary::new(&PackageId::new(name, "1.0.0", "http://www.example.com/").unwrap(),
                      &[])
     }
 
@@ -116,7 +116,7 @@ mod test {
 
     fn names(names: &[&'static str]) -> Vec<PackageId> {
         names.iter()
-            .map(|name| PackageId::new(*name, "1.0.0", "http://www.example.com/"))
+            .map(|name| PackageId::new(*name, "1.0.0", "http://www.example.com/").unwrap())
             .collect()
     }
 

diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs
index 86ce05f7297be452c798cb96b77fd9e7d3999ead..8776eac2898f44771635c3ab153970456ca2b975 100644 (file)
--- a/src/cargo/util/toml.rs
+++ b/src/cargo/util/toml.rs
@@ -88,13 +88,14 @@ pub struct TomlManifest {
 #[deriving(Decodable,Encodable,PartialEq,Clone,Show)]
 pub struct TomlProject {
     pub name: String,
+    // FIXME #54: should be a Version to be able to be Decodable'd directly.
     pub version: String,
     pub authors: Vec<String>,
     build: Option<String>,
 }
 
 impl TomlProject {
-    pub fn to_package_id(&self, namespace: &Url) -> PackageId {
+    pub fn to_package_id(&self, namespace: &Url) -> CargoResult<PackageId> {
         PackageId::new(self.name.as_slice(), self.version.as_slice(), namespace)
     }
 }
@@ -161,7 +162,7 @@ impl TomlManifest {
         let project = try!(project.require(|| human("No `package` or `project` section found.")));
 
         Ok((Manifest::new(
-                &Summary::new(&project.to_package_id(source_id.get_url()),
+                &Summary::new(&try!(project.to_package_id(source_id.get_url())),
                               deps.as_slice()),
                 targets.as_slice(),
                 &Path::new("target"),

diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs
index 57bd17dac9b14b2fe18e2a84433b373a67925f23..6354ad4221b75966651187bf560b1031a6f99ba6 100644 (file)
--- a/tests/test_cargo_compile.rs
+++ b/tests/test_cargo_compile.rs
@@ -51,6 +51,7 @@ test!(cargo_compile_with_invalid_manifest {
                       No `package` or `project` section found.\n"))
 })
 
+
 test!(cargo_compile_with_invalid_manifest2 {
     let p = project("foo")
         .file("Cargo.toml", r"
@@ -65,6 +66,23 @@ test!(cargo_compile_with_invalid_manifest2 {
                       Cargo.toml:3:19-3:20 expected a value\n\n"))
 })
 
+test!(cargo_compile_with_invalid_version {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            authors = []
+            version = "1.0"
+        "#);
+
+    assert_that(p.cargo_process("cargo-build"),
+                execs()
+                .with_status(101)
+                .with_stderr("Cargo.toml is not a valid manifest\n\n\
+                              invalid version: cannot parse '1.0' as a semver\n"))
+
+})
+
 test!(cargo_compile_without_manifest {
     let p = project("foo");